[Serve] HTTPS Support #3380
[Serve] HTTPS Support #3380
Conversation
cblmemo
force-pushed
the
serve-https-example-prototype
branch
from
030f39b to
56f2c04
Compare
|
Revamping this as a user is requesting the feature. Based on our previous discussion I disabled the update of the SSL, as it is not likely to update this - the most possible situation is the ssl file is not working and thus the service is failed anyway, so it should be fine to let users to restart the service. |
There was a problem hiding this comment.
Thanks for adding the example @cblmemo! Left several comments. Could we also add a smoke test for the HTTPS support?
We should also add a doc for this in another PR.
Also, does this mean the communication between load balancer and replicas is still going through non-secured HTTP, which might be fine for now, but would be good to mention it in comments or the doc.
|
Done! |
|
All Sky Serve smoke test passed and this is ready for another look @Michaelvll ! |
Doc added in #3972 |
There was a problem hiding this comment.
Thanks for adding the support of HTTPS @cblmemo! Left several comments : )
Also, we should do some backward compatibility test to make sure existing service won't be affected by this change.
sky/serve/core.py
Outdated
| @@ -127,6 +159,8 @@ def up( | |||
| controller_utils.maybe_translate_local_file_mounts_and_sync_up(task, | |||
| path='serve') | |||
|
|
|||
| tls_template_vars = _rewrite_tls_credential_paths(service_name, task) | |||
There was a problem hiding this comment.
Should we rename the function? Seems not actually rewrite?
| tls_template_vars = _rewrite_tls_credential_paths(service_name, task) | |
| tls_template_vars = _get_tls_template_vars(service_name, task) |
There was a problem hiding this comment.
Actually, we are rewriting the task.tls_credential in the function. Renamed to _rewrite_tls_credential_paths_and_get_tls_env_vars now.
| @@ -337,6 +341,7 @@ def _get_service_from_row(row) -> Dict[str, Any]: | |||
| 'requested_resources': pickle.loads(requested_resources) | |||
| if requested_resources is not None else None, | |||
| 'requested_resources_str': requested_resources_str, | |||
| 'tls_encrypted': bool(tls_encrypted), | |||
There was a problem hiding this comment.
For existing service, this return value will be None, and bool(tls_encrypted) can fail?
There was a problem hiding this comment.
Actually, bool(None) will return False:
python
Python 3.9.18 (main, Sep 11 2023, 13:41:44)
[GCC 11.2.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> bool(None)
FalseBut I agree that it is confusing. Changed it to 0 🫡
sky/serve/serve_state.py
Outdated
| @@ -68,6 +68,9 @@ def create_table(cursor: 'sqlite3.Cursor', conn: 'sqlite3.Connection') -> None: | |||
| db_utils.add_column_to_table(_DB.cursor, _DB.conn, 'services', | |||
| 'active_versions', | |||
| f'TEXT DEFAULT {json.dumps([])!r}') | |||
| # Whether the service's load balancer is encrypted with TLS. | |||
| db_utils.add_column_to_table(_DB.cursor, _DB.conn, 'services', 'tls_encrypted', | |||
| 'INTEGER DEFAULT NULL') | |||
There was a problem hiding this comment.
Should we add default value to existing entries?
There was a problem hiding this comment.
We already have a NULL default? Though i changed this to 0 to avoid confusion 🫡
|
@Michaelvll All comments resolved and now running the smoke test. PTAL agin! |
|
HTTPS smoke test passed! |
|
@Michaelvll PLAT, thx! |
Support HTTPS (SSL) Certificate on SkyServe Load Balancer.
Resolves (partially) #3198 . This PR only add TLS on load balancer - still need to figure out how to encrypt between LB and replica. #3458 might help this situation.
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.py --servepytest tests/test_smoke.py::test_skyserve_httpsbash tests/backward_comaptibility_tests.sh